fix: correct workspace admin permission validation in project member updates#9119
fix: correct workspace admin permission validation in project member updates#9119KanteshMurade wants to merge 3 commits into
Conversation
KanteshMurade
requested review from
dheeru0198 and
pablohashescobar
as code owners
📝 WalkthroughWalkthroughThis PR adds contract tests for project member role-update authorization. A test class with a URL helper is introduced, followed by three test cases validating that workspace admins can promote members and non-admins receive ChangesProject member role-update contract tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/plane/app/views/project/member.py (2)
209-230:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDuplicate workspace role queries with inconsistent variable names.
The code fetches
target_workspace_roleand the requester's workspace role twice:
- Lines 211-218:
requester_workspace_role/is_workspace_admin- Lines 220-229:
requesting_workspace_role/is_requesting_workspace_adminThis causes redundant database queries and creates confusion about which variables to use downstream. The HEAD conflict block uses
is_workspace_adminwhile the other block usesis_requesting_workspace_admin.After resolving the merge conflict, keep only one set of these queries with consistent naming.
Proposed fix (after resolving merge conflict)
- # Fetch the target's workspace role (used to cap the new project role) target_workspace_role = WorkspaceMember.objects.get( workspace__slug=slug, member=project_member.member, is_active=True ).role # Fetch the requester's workspace role to decide if they may bypass project-role checks requester_workspace_role = WorkspaceMember.objects.get( workspace__slug=slug, member=request.user, is_active=True ).role is_workspace_admin = requester_workspace_role == ROLE.ADMIN.value - - # Fetch the workspace role of the project member - target_workspace_role = WorkspaceMember.objects.get( - workspace__slug=slug, member=project_member.member, is_active=True - ).role - - # Fetch the workspace role of the requesting user for permission checks - requesting_workspace_role = WorkspaceMember.objects.get( - workspace__slug=slug, member=request.user, is_active=True - ).role - is_requesting_workspace_admin = requesting_workspace_role == ROLE.ADMIN.value -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/app/views/project/member.py` around lines 209 - 230, Remove the duplicated WorkspaceMember queries and normalize variable names: keep a single fetch for the target's workspace role into target_workspace_role (from WorkspaceMember with workspace__slug=slug and member=project_member.member) and a single fetch for the requesting user's workspace role into requester_workspace_role (from WorkspaceMember with workspace__slug=slug and member=request.user), then compute is_workspace_admin = requester_workspace_role == ROLE.ADMIN.value and use those variables throughout (replace any uses of requesting_workspace_role or is_requesting_workspace_admin with requester_workspace_role and is_workspace_admin) to eliminate redundant DB calls and make naming consistent.
246-293:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Unresolved merge conflict markers will cause syntax errors.
The file contains literal merge conflict markers (
<<<<<<< HEAD,=======,>>>>>>> c31299f8dd) that must be resolved before this code can run. Python will raise aSyntaxErrorwhen attempting to parse this file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/app/views/project/member.py` around lines 246 - 293, Resolve the literal merge markers by removing the conflict block and keeping a single coherent role-validation flow: delete the lines from <<<<<<< HEAD through ======= and the >>>>>>> marker, and retain the consolidated checks that first validate workspace limits using target_workspace_role and int(request.data.get("role", project_member.role)) against [15, 20], then validate the requester’s authority by comparing int(request.data.get("role", project_member.role)) to requested_project_member.role and using the consistent boolean name is_requesting_workspace_admin (or rename uses of is_workspace_admin to match). Ensure the code uses the same variable names (project_member vs requested_project_member) consistently, converts request.data role to int once, and returns the appropriate Response/status as in the consolidated version.
🧹 Nitpick comments (1)
apps/api/plane/tests/contract/app/test_project_app.py (1)
239-261: 💤 Low valueTest relies on implicit workspace admin setup from fixtures.
The test assumes
create_useris a workspace admin (role 20) through theworkspacefixture, but this isn't explicit in the test. If the fixture behavior changes, this test could fail unexpectedly.Consider adding an explicit assertion or comment documenting this fixture dependency:
# create_user is a workspace admin (role=20) via the workspace fixtureAlternatively, verify the workspace membership explicitly at the start of the test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/tests/contract/app/test_project_app.py` around lines 239 - 261, The test test_workspace_admin_can_promote_member_above_project_role relies implicitly on the create_user being a workspace admin via the workspace fixture; make that dependency explicit by asserting or ensuring the workspace membership/role at the start of the test (e.g., verify WorkspaceMember for create_user has role 20) or add a one-line comment documenting that create_user is a workspace admin; reference the test name test_workspace_admin_can_promote_member_above_project_role, the create_user fixture, and the WorkspaceMember/ProjectMember checks so maintainers can locate and make the verification or comment change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/plane/tests/contract/app/test_project_app.py`:
- Around line 283-284: The test in
apps/api/plane/tests/contract/app/test_project_app.py asserts
response.data["error"] equals "You cannot update a role that is higher than your
own role", which is one side of an unresolved merge in member.py (the
alternative message is "You cannot assign a role equal to or higher than your
own"); update the test to match the final error string chosen in member.py (or
make the assertion resilient by checking for the canonical substring used in the
resolved message) so the assertion aligns with the actual message thrown by the
role-update validation.
---
Outside diff comments:
In `@apps/api/plane/app/views/project/member.py`:
- Around line 209-230: Remove the duplicated WorkspaceMember queries and
normalize variable names: keep a single fetch for the target's workspace role
into target_workspace_role (from WorkspaceMember with workspace__slug=slug and
member=project_member.member) and a single fetch for the requesting user's
workspace role into requester_workspace_role (from WorkspaceMember with
workspace__slug=slug and member=request.user), then compute is_workspace_admin =
requester_workspace_role == ROLE.ADMIN.value and use those variables throughout
(replace any uses of requesting_workspace_role or is_requesting_workspace_admin
with requester_workspace_role and is_workspace_admin) to eliminate redundant DB
calls and make naming consistent.
- Around line 246-293: Resolve the literal merge markers by removing the
conflict block and keeping a single coherent role-validation flow: delete the
lines from <<<<<<< HEAD through ======= and the >>>>>>> marker, and retain the
consolidated checks that first validate workspace limits using
target_workspace_role and int(request.data.get("role", project_member.role))
against [15, 20], then validate the requester’s authority by comparing
int(request.data.get("role", project_member.role)) to
requested_project_member.role and using the consistent boolean name
is_requesting_workspace_admin (or rename uses of is_workspace_admin to match).
Ensure the code uses the same variable names (project_member vs
requested_project_member) consistently, converts request.data role to int once,
and returns the appropriate Response/status as in the consolidated version.
---
Nitpick comments:
In `@apps/api/plane/tests/contract/app/test_project_app.py`:
- Around line 239-261: The test
test_workspace_admin_can_promote_member_above_project_role relies implicitly on
the create_user being a workspace admin via the workspace fixture; make that
dependency explicit by asserting or ensuring the workspace membership/role at
the start of the test (e.g., verify WorkspaceMember for create_user has role 20)
or add a one-line comment documenting that create_user is a workspace admin;
reference the test name
test_workspace_admin_can_promote_member_above_project_role, the create_user
fixture, and the WorkspaceMember/ProjectMember checks so maintainers can locate
and make the verification or comment change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4476ba48-3f71-4f22-aac5-1839bc709b6e
📒 Files selected for processing (2)
apps/api/plane/app/views/project/member.pyapps/api/plane/tests/contract/app/test_project_app.py
KanteshMurade
force-pushed
the
clean-role-fix
branch
from
91e807d to
9f8572c
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/plane/app/views/project/member.py (1)
247-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't block all non-admin project members from role edits.
Lines 249-253 still short-circuit every requester whose project role is below
Admin, so the comparison-based checks below never run for members/guests. That means a project member cannot update someone to a lower role, and the new contract test inapps/api/plane/tests/contract/app/test_project_app.pywill currently get403 {"error": "You do not have permission to update roles"}instead of the intended “above your own role” denial.Suggested minimal fix
if "role" in request.data: - # Only Admins can modify roles - if requested_project_member.role < ROLE.ADMIN.value and not is_workspace_admin: - return Response( - {"error": "You do not have permission to update roles"}, - status=status.HTTP_403_FORBIDDEN, - ) - # Cannot modify a member whose role is equal to or higher than your own if project_member.role >= requested_project_member.role and not is_workspace_admin: return Response( {"error": "You cannot update the role of a member with a role equal to or higher than your own"}, status=status.HTTP_403_FORBIDDEN,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/app/views/project/member.py` around lines 247 - 253, The current check unconditionally blocks anyone below Admin from changing roles; change it to only block unauthorized promotions or edits of admins: when "role" in request.data, read the intended new role (new_role = request.data["role"]) and the requester's project role (e.g., requester_role), then if not is_workspace_admin enforce: if requester_role < ROLE.ADMIN.value and new_role > requester_role (attempting to promote above your role) OR if requested_project_member.role >= ROLE.ADMIN.value and requester_role < ROLE.ADMIN.value (non-admin trying to change an admin), return the 403; use ROLE enum values for comparisons and keep requested_project_member.role and is_workspace_admin checks in place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/api/plane/app/views/project/member.py`:
- Around line 247-253: The current check unconditionally blocks anyone below
Admin from changing roles; change it to only block unauthorized promotions or
edits of admins: when "role" in request.data, read the intended new role
(new_role = request.data["role"]) and the requester's project role (e.g.,
requester_role), then if not is_workspace_admin enforce: if requester_role <
ROLE.ADMIN.value and new_role > requester_role (attempting to promote above your
role) OR if requested_project_member.role >= ROLE.ADMIN.value and requester_role
< ROLE.ADMIN.value (non-admin trying to change an admin), return the 403; use
ROLE enum values for comparisons and keep requested_project_member.role and
is_workspace_admin checks in place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e16afcc6-902b-46d4-8a40-d6d9b9c086e5
📒 Files selected for processing (2)
apps/api/plane/app/views/project/member.pyapps/api/plane/tests/contract/app/test_project_app.py
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
This PR claims to fix #9109, but that bug was already fixed on preview via commit 4c1bdd1d62 (PR #9014), which made is_workspace_admin derive from the requester's workspace role. Tracing the test_workspace_admin_can_promote_member_above_project_role scenario against the base branch (no PR applied), the request already succeeds.
The new code on top introduces:
- A regression at
member.py:271(workspace-role cap now runs outsideif "role" in request.data, blocking non-role PATCHes on guests with elevated project roles). - Two duplicate
WorkspaceMember.objects.getqueries per call (lines 221, 226) —target_workspace_roleandrequesting_workspace_roleare re-fetches of the variables already computed two lines above. - An unreachable 400 branch at lines 277–285 (strictly weaker than the existing
>=guard above). - A silent permission expansion: removing the
requested_project_member.role < ROLE.ADMIN.valueguard lets non-admin project members modify guest project roles, where they were previously hard-blocked. Not mentioned in the PR description. - A failing test:
test_project_member_cannot_promote_member_above_own_project_rolewill fail — the earlierproject_member.role >= requested_project_member.roleguard returns 403 with a different message before the new 400 branch is reached.
Recommend either closing in favor of preview's current state, or rewriting to delete the duplicate queries/dead checks, move the workspace-role cap back inside if "role" in request.data, fix the failing test, and explicitly confirm whether the admin-only role-modification gate should be removed.
Inline comments below.
| is_workspace_admin = requester_workspace_role == ROLE.ADMIN.value | ||
|
|
||
| # Fetch the workspace role of the project member | ||
| target_workspace_role = WorkspaceMember.objects.get( |
There was a problem hiding this comment.
Duplicate query. Lines 211 (target_workspace_role) and 215 (requester_workspace_role) on the base branch already compute exactly this. Lines 221–229 re-fetch the same two WorkspaceMember rows and assign aliases (requesting_workspace_role, is_requesting_workspace_admin) for values that are bit-for-bit identical to requester_workspace_role / is_workspace_admin.
Net effect: 2 extra DB round-trips per partial_update and two pairs of synonymous variable names that will eventually diverge in someone's edit. Drop the duplicates and use the existing is_workspace_admin everywhere.
| {"error": "You do not have permission to update roles"}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Silent permission expansion. The deletion just above removed:
if requested_project_member.role < ROLE.ADMIN.value and not is_workspace_admin:
return Response({"error": "You do not have permission to update roles"},
status=status.HTTP_403_FORBIDDEN)That guard hard-blocked any non-admin requester from changing any role at all. Without it, a project MEMBER (role=15, not workspace admin) can now update a project GUEST's role — only the >= requested_project_member.role guard remains. Was this intentional? It's not mentioned in the PR description and isn't covered by either new test.
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) | ||
|
|
||
| if target_workspace_role in [5] and int(request.data.get("role", project_member.role)) in [15, 20]: |
There was a problem hiding this comment.
Regression — runs outside if "role" in request.data. This guard is no longer scoped to role updates. With request.data.get("role", project_member.role), when role is not in the request body, it falls back to the existing project_member.role.
Concrete failure: project member with workspace_role=GUEST (5) and pre-existing project_role=MEMBER (15) — e.g. legacy data, or anyone whose workspace role was downgraded after project assignment. Any PATCH that touches only non-role fields (preferences, sort order, view props, etc.) now 400s with "You cannot add a user with role higher than the workspace role".
Move this check back inside the if "role" in request.data: block — note the identical check already exists there at line 265, so this block is also redundant when role is present.
Minor: prefer [ROLE.GUEST.value] / [ROLE.MEMBER.value, ROLE.ADMIN.value] over the magic numbers [5] and [15, 20].
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) | ||
|
|
||
| if ( |
There was a problem hiding this comment.
Dead code. This 400 branch is unreachable:
- For a non-workspace-admin requester, the existing check at line 258 (
new_role >= requested_project_member.role and not is_workspace_admin) already returns 403 for anything strictly greater (>=subsumes>). It fires first. - For a workspace admin,
is_workspace_admin == is_requesting_workspace_admin == True(same expression, computed twice), sonot is_requesting_workspace_adminis False and this branch is skipped.
No input reaches the return Response(..., 400). Either delete it, or — if the intent was to surface a different error message — replace the earlier 403 instead of stacking another check after it.
| workspace__slug=slug, member=request.user, is_active=True | ||
| ).role | ||
| is_requesting_workspace_admin = requesting_workspace_role == ROLE.ADMIN.value | ||
|
|
There was a problem hiding this comment.
Trailing whitespace on this line (and stray double-blank lines at 209, 246, 287). Whatever the project lints with (ruff/flake8/black) will almost certainly flag these.
| """Non-workspace-admin project members cannot assign roles above their own project role.""" | ||
| project = Project.objects.create(name="Protected Role Project", identifier="PRP", workspace=workspace) | ||
|
|
||
| requesting_user = User.objects.create_user(email="requester@example.com", username="requester") |
There was a problem hiding this comment.
This assertion will fail.
With requester project_role=15 patching a target whose project_role=15, the first guard in partial_update fires:
if project_member.role >= requested_project_member.role and not is_workspace_admin:
return Response({"error": "You cannot update the role of a member with a role equal to or higher than your own"},
status=status.HTTP_403_FORBIDDEN)15 >= 15 is True, not False is True → returns 403 with the equal-to-or-higher message. The new 400 branch this test targets (member.py:277) is unreachable (see comment there).
Both assert response.status_code == 400 and the error string assertion will fail. To make the assertion match the new branch's message, the scenario needs the requester's project_role to be strictly greater than new_role for both prior guards — which contradicts the test name. Either redesign the scenario or assert against the actual 403 + message that the existing guard returns.
| requesting_project_member = ProjectMember.objects.create( | ||
| project=project, member=create_user, role=5, is_active=True | ||
| ) | ||
|
|
There was a problem hiding this comment.
This test passes on preview without the PR. The bug it documents (workspace admin with low project role unable to promote) was already fixed by #9014 (is_workspace_admin = requester_workspace_role == ROLE.ADMIN.value). The test is still valuable as a regression guard, but it shouldn't be framed as proving this PR fixes anything — the production fix is already shipped.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/plane/tests/contract/app/test_project_app.py (1)
240-264: 💤 Low valueConsider making the workspace admin setup explicit.
The test relies on
create_userbeing a workspace admin (via thesession_clientfixture setup) but doesn't explicitly establish this within the test body. Other tests in this file create workspace memberships explicitly (e.g., lines 578-579). While the test works as-is, adding an explicitWorkspaceMember.objects.create(workspace=workspace, member=create_user, role=ROLE.ADMIN.value, is_active=True)would make the test more self-documenting and reduce implicit fixture dependencies.📝 Suggested enhancement for clarity
def test_workspace_admin_can_promote_member_above_project_role(self, session_client, workspace, create_user): """Workspace admins can assign project roles above their own project role.""" project = Project.objects.create(name="Role Project", identifier="RP", workspace=workspace) + # Explicitly set up create_user as workspace admin + WorkspaceMember.objects.create( + workspace=workspace, member=create_user, role=ROLE.ADMIN.value, is_active=True + ) requesting_project_member = ProjectMember.objects.create( project=project, member=create_user, role=ROLE.GUEST.value, is_active=True )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/plane/tests/contract/app/test_project_app.py` around lines 240 - 264, The test test_workspace_admin_can_promote_member_above_project_role relies on an implicit fixture to make create_user a workspace admin; make this explicit by creating a WorkspaceMember for create_user with role=ROLE.ADMIN.value and is_active=True before performing the PATCH. Specifically, add a call to WorkspaceMember.objects.create(workspace=workspace, member=create_user, role=ROLE.ADMIN.value, is_active=True) near the start of the test (before the request) so the test clearly documents the requester’s admin status.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/api/plane/tests/contract/app/test_project_app.py`:
- Around line 240-264: The test
test_workspace_admin_can_promote_member_above_project_role relies on an implicit
fixture to make create_user a workspace admin; make this explicit by creating a
WorkspaceMember for create_user with role=ROLE.ADMIN.value and is_active=True
before performing the PATCH. Specifically, add a call to
WorkspaceMember.objects.create(workspace=workspace, member=create_user,
role=ROLE.ADMIN.value, is_active=True) near the start of the test (before the
request) so the test clearly documents the requester’s admin status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ecc6ac48-649b-407e-ac51-6dbda872038c
📒 Files selected for processing (1)
apps/api/plane/tests/contract/app/test_project_app.py
|
Thanks for the detailed review. After re-checking the current preview behavior and reviewing #9014, I realized the original issue had already been addressed and my additional permission logic changes were unnecessary. I’ve revised the PR to focus only on regression coverage:
The PR no longer changes backend permission behavior and is now intended only to validate and protect the current behavior. |
2 participants
Description
Fixes issue #9109 where workspace admins could not promote project members to Admin if their own project role was lower.
The backend permission validation in
ProjectMemberViewSet.partial_updatewas incorrectly checking the workspace role of the target member being edited instead of the requesting user.This change updates the validation logic to use the requesting user's workspace role for permission checks.
Type of Change
Screenshots and Media (if applicable)
N/A
Test Scenarios
References
Fixes #9109
Summary by CodeRabbit